Skip to content

fix: stack clip regions so nested clips restore the parent rect#85

Open
natemoo-re wants to merge 5 commits into
mainfrom
nm/repro/nested-clip
Open

fix: stack clip regions so nested clips restore the parent rect#85
natemoo-re wants to merge 5 commits into
mainfrom
nm/repro/nested-clip

Conversation

@natemoo-re

@natemoo-re natemoo-re commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Member
  • Fixes 🐛 Nested clip regions leak (clip is a single rect, not a stack) #77
  • Clip state was a single rect, so closing an inner clip turned clipping off entirely and later siblings leaked past the outer bounds (both vertically and horizontally)
  • Replaces the scalar clip rect with a fixed-depth stack (16): SCISSOR_START pushes intersect(parent, child), SCISSOR_END pops and restores the parent rect while the stack is non-empty
  • The active top mirrors into the existing clipx/clipy/clipw/cliph scalars, so setcell is untouched; depth-1 nesting reduces to the previous behavior; stack depth resets each frame
  • The regression lives in test/clip.test.ts under describe("clip") rather than a standalone file
  • Test plan: deno test green (8 passed); deno lint + deno fmt --check clean

@pkg-pr-new

pkg-pr-new Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Open in StackBlitz

npm i https://pkg.pr.new/clayterm@85

commit: 32deb34

@github-actions

github-actions Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown

Size Increased — +0.7 KB

111.0 KB unpacked

@natemoo-re natemoo-re force-pushed the nm/repro/nested-clip branch from 4300a52 to 4b35dc4 Compare June 5, 2026 06:14
@codspeed-hq

codspeed-hq Bot commented Jun 5, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 5 untouched benchmarks
⏩ 18 skipped benchmarks1

Comparing nm/repro/nested-clip (32deb34) with main (72890de)

Open in CodSpeed

Footnotes

  1. 18 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@cowboyd

cowboyd commented Jun 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Collaborator

@natemoo-re This is a great addition. I've made a few tweaks:

  1. Visual tests. I've found that when investigating as a human, having a visual test for layout where possible really helps our little brains hone in on the context rapidly, so I proposed a few tests that accomplish the same thing, but instead of asserting against bytes, assert agains pictures.
  2. Specs To develop clayterm, I've been using spec-oriented development. I've found that it helps agents keep really precise project-wide context when making micro-code decisions. My usual workflow is research -> spec -> test -> implementation. I've updated the spec in this case, let me know what you think.

As for feedback, This seems like a very necessary addittion. The thing that I added to the spec that isn't currently present in the implementation is the behavior when we overflow the clip stack. Right now, it will silently corrupt every command that comes after the overflow. I've specified the behavior to add the constraint that even in overflow, the stack must be balanced, and also that in the event of overflow, it must be signaled back via the error channel that overflow occurred so that the front end can issue a warning.

In practice, I have no idea what kind of UI that would in any way be usable would also have a clip stack of 16, but hey 🤷🏻

Lemme know what you think!

@cowboyd

cowboyd commented Jun 6, 2026

Copy link
Copy Markdown
Collaborator

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Simulation long input burst (200 bytes) 576.1 µs 1,264.9 µs -54.46%

Highly suspicious since this is an input parser benchmark, and the only code changes were to the render pipeline

natemoo-re and others added 5 commits June 16, 2026 22:33
@natemoo-re
test: failing repro for nested-clip
f70efa7
@natemoo-re
fix: stack clip regions so nested clips restore the parent rect
4f210f2 
Clip state was a single rect, so closing an inner clip turned
clipping off entirely and later siblings leaked past the outer
bounds. Replace it with a fixed-depth stack: SCISSOR_START pushes
intersect(parent, child) and SCISSOR_END pops, restoring the parent
rect while the stack is non-empty. The active top mirrors into the
existing clipx/clipy/clipw/cliph scalars, so setcell is unchanged.

Fixes #77
@natemoo-re
test: move nested-clip regression into test/clip.test.ts
f2526c9
@cowboyd @natemoo-re
🌆 Use visual language for clipping tests
b47c1ed
@cowboyd @natemoo-re
📝 Update render spec to include clipping behavior
32deb34
@natemoo-re natemoo-re force-pushed the nm/repro/nested-clip branch from b5a355d to 32deb34 Compare June 17, 2026 02:47
@natemoo-re natemoo-re requested a review from cowboyd June 17, 2026 15:21
@cowboyd

cowboyd commented Jun 17, 2026

Copy link
Copy Markdown
Collaborator

Noting this up here: there is an upstream issue and draft PR for having Clay track the clip stack natively. We should track this and we can remove our own implementation when this lands upstream.

cowboyd
cowboyd previously approved these changes Jun 17, 2026
View reviewed changes
@cowboyd cowboyd self-requested a review June 17, 2026 20:44
@cowboyd cowboyd dismissed their stale review June 17, 2026 20:44

Still needs to implement the clip stack symmetry in the spec

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cowboyd cowboyd Awaiting requested review from cowboyd

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

🐛 Nested clip regions leak (clip is a single rect, not a stack)

2 participants

@natemoo-re @cowboyd